Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Transform] Transforms health alerting rule type #112277

Merged
merged 34 commits into from
Oct 6, 2021
Merged

[Transform] Transforms health alerting rule type #112277

merged 34 commits into from
Oct 6, 2021

Conversation

darnautov
Copy link
Contributor

@darnautov darnautov commented Sep 15, 2021 •
edited
Loading

Summary

Part of #111945

image

Checklist

@darnautov darnautov added :ml v8.0.0 Feature:Transforms ML transforms release_note:feature Makes this part of the condensed release notes auto-backport Deprecated - use backport:version if exact versions are needed v7.16.0 labels Sep 15, 2021
@darnautov darnautov self-assigned this Sep 15, 2021
peteharverson
peteharverson reviewed Sep 15, 2021
View reviewed changes
...ugins/transform/public/alerting/transform_health_rule_type/register_transform_health_rule.ts Outdated Show resolved Hide resolved
...ugins/transform/public/alerting/transform_health_rule_type/transform_health_rule_trigger.tsx Show resolved Hide resolved
...ugins/transform/public/alerting/transform_health_rule_type/transform_health_rule_trigger.tsx Outdated Show resolved Hide resolved
...ugins/transform/public/alerting/transform_health_rule_type/transform_health_rule_trigger.tsx Outdated Show resolved Hide resolved
.../plugins/transform/public/alerting/transform_health_rule_type/transform_selector_control.tsx Outdated Show resolved Hide resolved
x-pack/plugins/transform/server/lib/alerting/register_transform_alerting_rules.ts Outdated Show resolved Hide resolved
@darnautov darnautov requested a review from lcawl October 4, 2021 11:58
matschaffer
matschaffer reviewed Oct 5, 2021
View reviewed changes
Copy link
Contributor

@matschaffer matschaffer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems generally good to me. It might be good for @mgiota or @miltonhultgren to cross check given they've been doing more in the infra UI alerting space.

x-pack/plugins/monitoring/server/plugin.ts Outdated
@@ -267,6 +271,8 @@ export class MonitoringPlugin
}

registerPluginInUI(plugins: PluginsSetup) {
const alertingRules = [...RULES, TRANSFORM_RULE_TYPE.TRANSFORM_HEALTH];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason we're not adding TRANSFORM_RULE_TYPE.TRANSFORM_HEALTH to the RULES constant directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried it at first but RULES are used by the factory here. So it requires extending the BaseRule class which I assume is redundant for the Transform alerting rule.

@darnautov darnautov requested a review from a team as a code owner October 5, 2021 12:37
@darnautov
Copy link
Contributor Author

@elasticmachine merge upstream

YulNaumenko
YulNaumenko reviewed Oct 5, 2021
View reviewed changes
x-pack/plugins/monitoring/server/plugin.ts Outdated
@@ -128,6 +130,8 @@ export class MonitoringPlugin
for (const alert of alerts) {
plugins.alerting?.registerType(alert.getRuleType());
}
plugins.alerting?.registerType(getTransformHealthRuleType());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any specific reason to do not use a factory?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We discussed it and realized it's not a right fit for the Stack Monitoring rules. We're going to include it in the Stack Rules instead.

YulNaumenko
YulNaumenko reviewed Oct 6, 2021
View reviewed changes
Copy link
Contributor

@YulNaumenko YulNaumenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should update the rules telemetry list x-pack/plugins/alerting/server/usage/alerts_usage_collector.ts as well and create a proper update in the telemetry repository, for example: https://github.com/elastic/telemetry/pull/530

@darnautov darnautov requested a review from a team as a code owner October 6, 2021 12:56
@darnautov
Copy link
Contributor Author

thanks, @YulNaumenko, I've updated mappings in de0a7d9. Also realized there were some issues with the ML related mappings 😓

@darnautov darnautov requested a review from YulNaumenko October 6, 2021 12:59
YulNaumenko
YulNaumenko approved these changes Oct 6, 2021
View reviewed changes
Copy link
Contributor

@YulNaumenko YulNaumenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
transform 227 235 +8

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
transform 0 4 +4

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
transform 308.7KB 322.1KB +13.4KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
transform 0 1 +1

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
core 299.3KB 299.4KB +78.0B
transform 12.2KB 15.5KB +3.3KB
total +3.4KB
Unknown metric groups

API count

id before after diff
transform 0 4 +4

async chunk count

id before after diff
transform 1 3 +2

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @darnautov

@darnautov darnautov merged commit 6da1323 into elastic:master Oct 6, 2021
@darnautov darnautov deleted the transform-111945-health-alerting-rule branch October 6, 2021 16:27
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Oct 6, 2021
@darnautov @kibanamachine
[Transform] Transforms health alerting rule type (elastic#112277)
f56d904
@kibanamachine kibanamachine mentioned this pull request Oct 6, 2021
Merged
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
✅ 7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Oct 6, 2021
@kibanamachine @darnautov
[Transform] Transforms health alerting rule type (#112277) (#114142)
2875113 
Co-authored-by: Dima Arnautov <[email protected]>
This was referenced Oct 7, 2021
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@szabosteve szabosteve szabosteve approved these changes

@matschaffer matschaffer matschaffer left review comments

@YulNaumenko YulNaumenko YulNaumenko approved these changes

@qn895 qn895 qn895 approved these changes

@peteharverson peteharverson peteharverson approved these changes

@pgayvallet pgayvallet pgayvallet approved these changes

@lcawl lcawl Awaiting requested review from lcawl

Assignees

@darnautov darnautov

Labels
auto-backport Deprecated - use backport:version if exact versions are needed Feature:Transforms ML transforms :ml release_note:feature Makes this part of the condensed release notes v7.16.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@darnautov @elasticmachine @qn895 @kibanamachine @matschaffer @pgayvallet @peteharverson @szabosteve @lcawl @YulNaumenko